fix(utils): use max-enum-value+1 as checkHasKey boundary; reject empty keys - #22
Draft
bitgo-ai-agent-dev[bot] wants to merge 2 commits into
Conversation
added 2 commits
August 7, 2026 19:54
addOutput() was calling addUnknownKeyValToInput(outputIndex, keyVal)
instead of addUnknownKeyValToOutput(outputIndex, keyVal). This caused
two confirmed failure modes:
1. Silent corruption when outputIndex < inputs.length: the keyval was
attached to inputs[outputIndex].unknownKeyVals and survived
serialize/parse round-trips in the wrong (input) map. Because the
duplicate-check also ran against InputTypes (16 entries) instead of
OutputTypes (6 entries), output unknown-key type bytes 6–15 were
additionally misrejected.
2. Crash ("No input #N") when outputIndex >= inputs.length — the
common 1-input/2-output case — making PSBT construction impossible
whenever unknownKeyVals accompany the second output.
Both modes affect musig2/MPC coordination data carried as output
proprietary fields: the signing ceremony crashes or corrupts the PSBT,
leaving funds stuck in the shared wallet.
Fix: change line 159 to call addUnknownKeyValToOutput, matching the
upstream bip174 v2.1.1 fix. Add regression tests covering correct
placement, round-trip survival, and the outputs>inputs case.
Ticket: WCN-1934
Session-Id: 9abd2e08-b701-4f8d-9355-06124e17bf0c
Task-Id: edef13cc-a93b-4c12-85ac-d51bcc1dfaaa
…y keys Two correctness bugs in checkHasKey / getEnumLength exposed by the addOutput routing fix (WCN-1934 primary fix): 1. getEnumLength counted named enum members rather than returning max-numeric-value + 1. For non-contiguous enums this diverges: OutputTypes has 6 named members but its highest type byte is 7 (TAP_BIP32_DERIVATION = 0x07). InputTypes has 16 named members but its highest type byte is 24 (TAP_MERKLE_ROOT = 0x18 = 24). The old count-based threshold let TAP_TREE (0x06) and TAP_BIP32_DERIVATION (0x07) bypass the "use the typed method" guard on the output path, and let all six taproot input types (0x13-0x18) bypass the same guard on the input path. A caller could silently store these known fields in unknownKeyVals alongside their typed counterparts, producing a PSBT with duplicate-key serialization that conformant parsers reject. Fix: derive the threshold as max(numeric enum values) + 1. 2. A zero-length key Buffer passed checkHasKey unchecked: key[0] is undefined, and undefined < N is false in JavaScript, so the guard did not throw. The zero-length key was stored in unknownKeyVals, then serialized as the PSBT end-of-map separator byte (0x00), causing silent data loss on round-trip without any error at write time. Fix: explicit key.length === 0 guard at the top of checkHasKey. Adds 7 regression tests covering: - key byte 0x08 (above OutputTypes max) is accepted as unknown - key bytes 0x06 / 0x07 (TAP_TREE / TAP_BIP32_DERIVATION) are rejected - zero-length key is rejected on output, input, and global paths Ticket: WCN-1934 Session-Id: 9abd2e08-b701-4f8d-9355-06124e17bf0c Task-Id: edef13cc-a93b-4c12-85ac-d51bcc1dfaaa
ralph-bitgo
Bot
force-pushed
the
fix/WCN-1934-addOutput-unknownKeyVals-routing-pt2
branch
from
August 7, 2026 20:03
e938561 to
ba0680a
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ts_src/lib/utils.ts—getEnumLength: rewrite to returnmax(numeric enum values) + 1instead of counting named members.For
OutputTypesthis changes the threshold from 6 → 8 (coveringTAP_TREE = 0x06andTAP_BIP32_DERIVATION = 0x07); forInputTypesfrom 16 → 25 (covering all taproot input types
0x13–0x18).ts_src/lib/utils.ts—checkHasKey: add explicit guard rejectingzero-length
keyBuffers before the type-byte comparison.ts_src/tests/addInputOutput.ts— add 7 new regression tests:0x08(aboveOutputTypesmax) is accepted as unknown.0x06/0x07(TAP_TREE/TAP_BIP32_DERIVATION) are now rejected.Why
Two correctness bugs in
checkHasKey/getEnumLengthwere exposed whenthe pt1 routing fix made the output path reachable:
Bug 1 — wrong threshold for non-contiguous enums (WCN-1934):
getEnumLengthcounted named enum members rather than returningmax-value + 1.OutputTypeshas 6 named members but a highest type byteof
0x07(TAP_BIP32_DERIVATION), soTAP_TREE (0x06)andTAP_BIP32_DERIVATION (0x07)bypassed the "use the typed method" guard(since
6 < 6and7 < 6are both false). A caller could silently storethese known fields in
unknownKeyValsalongside their typed counterparts,producing a PSBT with duplicate-key serialization that conformant parsers
reject. The same flaw affected the input path: all six taproot input types
(
0x13–0x18) had values above the old threshold of 16 and also escaped.Bug 2 — zero-length key causes silent round-trip data loss:
key[0]isundefinedfor a zero-length Buffer;undefined < Nisfalsein JavaScript, so the guard passed silently. The zero-length key was then
stored and serialized as the PSBT end-of-map separator byte (
0x00),causing silent data loss on
fromBufferre-parse with no error thrown.Test plan
npm run unit— 394 tests pass (7 new regression tests added)npm run format:ci— no formatting issuesStack
This PR is part 2 of 2 in a stack. Review and merge in order:
master)checkHasKey/getEnumLengthcorrectness fixes (base:pt1) ← you are hereTicket: WCN-1934